http conn man: fix memory leak with watermark buffer#23401
Conversation
Fix memory leak with stream recreated when watermark buffer tracking enabled. Call clearDownstream() method for the old stream's memory account inside ConnectionManagerImpl::ActiveStream::recreateStream to unregister old account before creating a new one. Signed-off-by: Artur Malchanau <artur.molchanov@bolt.eu>
|
Hi @Hexta, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
/assign @KBaichoo |
KBaichoo
left a comment
KBaichoo
left a comment
There was a problem hiding this comment.
Thanks for catching this! Few improvements I think we can make.
| // Make sure to not check for deferred close as we'll be immediately creating a new stream. | ||
| connection_manager_.doEndStream(*this, /*check_for_deferred_close*/ false); | ||
|
|
||
| auto oldAccount = response_encoder->getStream().account(); |
There was a problem hiding this comment.
can we follow style guide: https://google.github.io/styleguide/cppguide.html#Variable_Names for naming local variable
| if (oldAccount != nullptr) { | ||
| oldAccount->clearDownstream(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The slices in the request_data still are referring to the old account, we should have them transfered to refer to the account of the recreated stream (crediting the old account which will be destroyed at the end of this function)
There was a problem hiding this comment.
Probably, we can re-use the old account because the downstream connection is still the same.
WDYT?
There was a problem hiding this comment.
yeah I think that'd work and be simpler. We would need to modify ConnectionManagerImpl::newStream for the case there's an existing account associated which there will be if we're recreating the stream.
| } | ||
|
|
||
| if (streamBufferAccounting()) { | ||
| EXPECT_EQ(buffer_factory_->numAccountsCreated(), 2); |
There was a problem hiding this comment.
can we validate the transfer of bytes in request body so that we include them in our accounting? (possibly in another test, this one has no request body).
|
/wait |
Keep the old memory account for the recreated stream to simplify logic. Add an integration test for internal redirect with request body. Signed-off-by: Artur Malchanau <artur.molchanov@bolt.eu> Signed-off-by: Artur Malchanau <artur.molchanov@taxify.eu>
KBaichoo
left a comment
KBaichoo
left a comment
There was a problem hiding this comment.
Just minor changes, otherwise pending CI this looks good. Thanks for catching this.
| default_request_headers_.setHost("handle.internal.redirect"); | ||
| default_request_headers_.setMethod("POST"); | ||
|
|
||
| const std::string& request_body = "foobarbizbaz"; |
There was a problem hiding this comment.
this shouldn't be a reference
| } | ||
|
|
||
| TEST_P(Http2BufferWatermarksTest, AccountsAndInternalRedirectWithRequestBody) { | ||
| Http::TestResponseHeaderMapImpl redirect_response_{ |
There was a problem hiding this comment.
nit: naming, should be redirect_response the _ suffix is reserved for object fields. Consider making const since it's not modified and shifting it down right where it's used.
Same with above test.
|
/wait |
Signed-off-by: Artur Malchanau <artur.molchanov@bolt.eu>
|
/retest |
|
Retrying Azure Pipelines: |
3 participants
Fix memory leak with stream recreated when watermark buffer tracking enabled.
Do not create a new memory account for the recreated stream in ConnectionManagerImpl::ActiveStream::recreateStream.
Re-use an account from the old stream.
Risk Level: low
Testing: integration